Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(rebuild): re-incorporate redirects #2034

Merged
merged 4 commits into from
Apr 17, 2018
Merged

chore(rebuild): re-incorporate redirects #2034

merged 4 commits into from
Apr 17, 2018

Conversation

EugeneHlushko
Copy link
Member

@EugeneHlushko EugeneHlushko commented Apr 13, 2018

Re-intergrate redirects

Note: i took latest redirects list from master

@montogeek
Copy link
Member

What happened to the redirects.json file?

@skipjack skipjack changed the title feature(redirects) Re-incorporate redirects chore(rebuild): re-incorporate redirects Apr 15, 2018
Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me and I love how we've isolated redirects to a single location. The only concern I'd have is how redirects are handled within the site. Meaning that because these are server-side files that will be output, once you've entered the site and are in SPA mode, these won't be hit.

Now that I think about it though, maybe that's a non-issue. Any links we have control over should just be updated, and these are mainly for external sites linking back here. Once the conflict is addressed I think this is good to go.

DISCUSSION.md Outdated
@@ -13,7 +13,7 @@ this branch:
- [ ] Extract anchors into `_content.json` via `DirectoryTreePlugin`
- [ ] Finish re-incorporating mobile sidebar
- [ ] Re-integrate google-analytics
- [ ] Re-incorporate `redirects.json`
- [x] Re-incorporate `redirects.json` (Eugene)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you drop this change to fix the conflict? I removed DISCUSSION.md in favor of github threads (i.e. PRs and issues). It wasn't meant to be a long-lived document anyway, just gave some perspective on various issues.

@Munter
Copy link
Collaborator

Munter commented Apr 15, 2018

When the hyperlink branch merges in you won't have any links pointing at moved pages any more. I'm also pretty sure we already addressed all of them on master, but if there are any left, rest assured you'll have automation to find them

@EugeneHlushko
Copy link
Member Author

EugeneHlushko commented Apr 15, 2018

@skipjack its done
@montogeek redirects.json is getting removed, somehow it wasn't commited at first

EugeneHlushko and others added 4 commits April 17, 2018 01:38
Including this plugin in `common` causes it to be run twice for production builds as both
parallelized configs pick this up. We only need it in `prod` and it's kind of nice to have
all html generation happen in one build so lumping it together with the `SSGPlugin` makes
sense.
@skipjack
Copy link
Collaborator

@EugeneHlushko hope you don't mind, I tested this out before merging and noticed that plugin was actually run twice because of our parallelized prod configs. By moving it next to the SSGPlugin, we only run it once and lump together all HTML generation into a single build.

I'm going to merge, but if you see any issue with this change don't hesitate to comment.

@skipjack skipjack merged commit 708c776 into rebuild Apr 17, 2018
@skipjack skipjack deleted the feature/redirect branch April 17, 2018 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants